Fixing cluster stats response #4632
Conversation
|
Following you can find the validation changes against the target branch for the APIs.
You can validate these APIs yourself by using the |
|
nodes.stats will also be fixed, but this one and the other one need to be merged first |
There was a problem hiding this comment.
In SegmentStats, I'm seeing (existing) fields that I would not expect:
- index_writer_max_memory_in_bytes?: long
- stored_memory?: ByteSize
In StatsResponseBase:
statusappears to be optional- should we add ESQL metrics?
In ClusterFileSystem, there are other optional fields
Still need to finish reviewing types.ts and Stats.ts.
nice catch, removing those
status is optional yes, while
adding them, thank you! |
There was a problem hiding this comment.
In ClusterIndices, mappings is optional, as well as analysis
In FieldTypesMappings, total_field_count and total_deduplicated_field_count are long.
In FieldTypes, indexed_vector_count is a long. However, should we call it DenseVectorFieldStats? FieldStats is a different child class in the Java code.
All fields built with humanReadableField are marked optional, but I don't understand why when reading the code.
specification/cluster/stats/types.ts
Outdated
| export class DenseVectorOffHeapStats { | ||
| total_size_bytes?: long | ||
| total_size?: ByteSize | ||
| total_veb_size_bytes?: long | ||
| total_veb_size?: ByteSize | ||
| total_vec_size_bytes?: long | ||
| total_vec_size?: ByteSize | ||
| total_veq_size_bytes?: long | ||
| total_veq_size?: ByteSize | ||
| total_vex_size_bytes?: long | ||
| total_vex_size?: ByteSize | ||
| } |
There was a problem hiding this comment.
Thanks! I'm assuming you haven't seen https://github.com/elastic/elasticsearch/blob/c8e719544a9228c6b87a0d23100d478411101656/server/src/main/java/org/elasticsearch/index/shard/DenseVectorStats.java#L140-L142 do anything in practice?
There was a problem hiding this comment.
ah thanks I forgot about this one! I'm not sure how to map it since it's all custom field values, let's discuss it with the team
|
@pquentin thanks for the review! I think I addressed everything |
|
nodes.stats is next |
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ddc225d)
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ddc225d)
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ddc225d)
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ddc225d)
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- (cherry picked from commit ddc225d) Co-authored-by: Laura Trotta <[email protected]> Co-authored-by: Quentin Pradet <[email protected]>
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- (cherry picked from commit ddc225d) Co-authored-by: Laura Trotta <[email protected]> Co-authored-by: Quentin Pradet <[email protected]>
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- (cherry picked from commit ddc225d) Co-authored-by: Laura Trotta <[email protected]> Co-authored-by: Quentin Pradet <[email protected]>
* fixing cluster stats response * addressing review with more fixes * update healthstatus enums * addressing review * add fielddata, more comments * fix type dense vector fields * fixed raw values not nullable --------- (cherry picked from commit ddc225d) Co-authored-by: Laura Trotta <[email protected]> Co-authored-by: Quentin Pradet <[email protected]>
From 0/27 validated responses to 27/27. Providing server side proof for each of these changes would be a lot, most of these changes are adding the
in_bytes(or not in_bytes) alternative for fields and making both optional.New classes:
GlobalOrdinalsStats-> server codeClusterSnapshotStats-> server codeSearchUsageStats-> server codeDenseVectorStats-> server codeSparseVectorStats-> server codeSince it was broken before I think this can all be backported safely
About FieldTypes: In the server code there is a parent class
IndexFeatureStatswhich is extended byDenseVectorFieldStats, the specific class to hold dense vector related fields. Currently we cannot have the same distinction, because it would be one of those unions which are hard to deserialize (no unique subset of fields) and also it would introduce breaking changes.